Pass weights to wrr balancer through attributes. #3530

easwars · 2020-04-15T00:28:07Z

Add methods to the weightedroundrobin package to add/get AddrInfo to/from attributes.Attributes.
Update the edsBalancer implementation to send the weights as attributes using these methods.
Update attributes.WithValues to work with a nil receiver to avoid call sites from having to perform nil checks and creating empty Attributes before being able to use this method.

#effectively-use-attributes

easwars · 2020-04-15T00:29:16Z

Not sure if I should mark a milestone for this PR.
@dfawley @menghanl

menghanl · 2020-04-15T21:45:50Z

balancer/weightedroundrobin/weightedwoundrobin_test.go

+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+


Intellij seems to be doing this. I have been trying to delete them wherever I notice.

menghanl · 2020-04-15T21:46:26Z

balancer/weightedroundrobin/weightedroundrobin.go

+
+	// Attribute key used to store AddrInfo in the Attributes field of
+	// resolver.Address.
+	attributeKey = "/balancer/weightedroundrobin/addrInfo"


Make a type, so only this package can get/set the value?

Done. Thanks.

menghanl · 2020-04-15T21:47:26Z

balancer/weightedroundrobin/weightedroundrobin.go

+
+// AddAddrInfoToAttributes returns a new Attributes containing all key/value
+// pairs in a with ai being added to it.
+func AddAddrInfoToAttributes(ai *AddrInfo, a *attributes.Attributes) *attributes.Attributes {


Should this take an address and return an address?

And this is not "add", this is "set", because it overrides the previous info, if there's one.

menghanl · 2020-04-16T18:13:48Z

balancer/weightedroundrobin/weightedroundrobin.go

 type AddrInfo struct {
 	Weight uint32
 }
+
+// SetAddrInfo sets addInfo in the Attributes field of addr.
+func SetAddrInfo(addrInfo *AddrInfo, addr *resolver.Address) {


There's a discussion on whether the function should take a pointer and modify, or make a copy: #3494 (comment). Let's wait for that.

And make the function experimental.

Please change these to not be pointers - do copies instead. Since these are not performance sensitive, we will optimize for code clarity and proper usage, which copying would better provide, I believe. E.g. using a pointer means modifications to the AddrInfo passed in will affect addresses which received a pointer to it in their attributes. This could lead to bugs.

dfawley · 2020-04-16T21:19:16Z

attributes/attributes.go

@@ -50,6 +50,9 @@ func New(kvs ...interface{}) *Attributes {
 // times, the last value overwrites all previous values for that key.  To
 // remove an existing key, use a nil value.
 func (a *Attributes) WithValues(kvs ...interface{}) *Attributes {
+	if a == nil {
+		a = New()


return New(kvs).

Note that Value doesn't handle a nil a.

Done for both.

dfawley · 2020-04-16T21:28:11Z

balancer/weightedroundrobin/weightedroundrobin.go

 type AddrInfo struct {
 	Weight uint32
 }
+
+// SetAddrInfo sets addInfo in the Attributes field of addr.
+func SetAddrInfo(addrInfo *AddrInfo, addr *resolver.Address) {


Please change these to not be pointers - do copies instead. Since these are not performance sensitive, we will optimize for code clarity and proper usage, which copying would better provide, I believe. E.g. using a pointer means modifications to the AddrInfo passed in will affect addresses which received a pointer to it in their attributes. This could lead to bugs.

easwars

PTAL.

easwars · 2020-04-16T23:18:28Z

balancer/weightedroundrobin/weightedroundrobin.go

 type AddrInfo struct {
 	Weight uint32
 }
+
+// SetAddrInfo sets addInfo in the Attributes field of addr.
+func SetAddrInfo(addrInfo *AddrInfo, addr *resolver.Address) {


easwars · 2020-04-16T23:22:02Z

attributes/attributes.go

@@ -50,6 +50,9 @@ func New(kvs ...interface{}) *Attributes {
 // times, the last value overwrites all previous values for that key.  To
 // remove an existing key, use a nil value.
 func (a *Attributes) WithValues(kvs ...interface{}) *Attributes {
+	if a == nil {
+		a = New()


Done for both.

dfawley · 2020-04-17T16:01:30Z

balancer/weightedroundrobin/weightedroundrobin.go

 type AddrInfo struct {
 	Weight uint32
 }
+
+// SetAddrInfo sets addInfo in the Attributes field of addr.


"sets addrInfo"

dfawley · 2020-04-17T16:02:52Z

balancer/weightedroundrobin/weightedroundrobin.go

+
+// SetAddrInfo sets addInfo in the Attributes field of addr.
+// This is an EXPERIMENTAL API.
+func SetAddrInfo(addrInfo AddrInfo, addr *resolver.Address) {


Please update this to return a resolver.Address and not accept a pointer.

dfawley · 2020-04-17T16:04:18Z

balancer/weightedroundrobin/weightedroundrobin.go

+// GetAddrInfo returns the AddrInfo stored in the Attributes fields of addr.
+// Returns nil if no AddrInfo is present.
+// This is an EXPERIMENTAL API.
+func GetAddrInfo(addr *resolver.Address) AddrInfo {


Also, don't accept a pointer here for symmetry and simplicity. Performance is not a concern here.

dfawley · 2020-04-17T16:05:20Z

xds/internal/balancer/edsbalancer/eds_impl.go

+				// attributes about the address. We still need to populate the
+				// Metadata field here to allow users of this field to migrate
+				// to the new one.
+				// TODO(easwars): Remove this once all users have migrated.


Is this behavior documented? Who are the users you're referring to? Only us?

@menghanl mentioned that dropbox was a user.

I'm a little lost. This is the address we're passing to the weighted round robin balancer - why do we need to set Metadata for it? Are they using xds and overriding our weighted RR balancer?

I'm not very sure how (or for what) they use it. @menghanl might have an answer.

Yes. And they use edf wrr #2968

@menghanl How will we track the safe removal of this?

Keep it for one more release, and remove after 1.30?

That's fine with me if it's OK with dropbox. Please file an issue and cc them?

Issue filed #3563

dfawley · 2020-04-17T22:42:21Z

attributes/attributes.go

+// Clone returns a new Attributes containing all key/value pairs found in a.
+// Since Attributes stores its key/value paris as type `interface{}`, it is not
+// possible to perform deep copies here.
+func (a *Attributes) Clone() *Attributes {


Attributes are immutable, so if we can't do a deep copy, I don't think there's any value of a Clone(), is there?

WithValues() is already making a clone.

I guess what I wanted to say was that since Attributes stores key/value pairs of type interface{}, one can store a pointer as the value type. The caller of the Clone operation can then modify the contents of the data pointed to by the value. This is no different from what WithValues does. So, maybe I should remove that comment about the deep copy.

In fact I could and maybe i should replace the implementation of Clone as follows:

func (a *Attributes) Clone() *Attributes { return a.WithValues() }

But why have Clone at all? Or call Attributes.WithValues with no parameters? There is no way to modify a *Attributes, so there is no need to Clone it. I.e.:

a := attributes.New(kvs...) b := a.WithValues(otherKVs...) c := a.WithValues(otherOtherKVs...)

a, b, and c will all have kvs, b will be the only thing with otherKVs, and c will be the only one with otherOtherKVs. You can safely pass around and make copies of *Attributes.

If a user stores a pointer in *Attributes and modifies it later, Clone-ing the *Attributes won't change the fact that modification to that pointer will modify the *Attributes (clones and the original). The only way to prevent that would have been to force the values stored in Attributes to have a Clone method on them, and call that in WithValues/Clone.

Makes sense, got rid of Clone.

easwars · 2020-04-21T18:27:18Z

Ping ...

dfawley · 2020-04-22T17:00:54Z

balancer/weightedroundrobin/weightedroundrobin.go

+//
+// This is an EXPERIMENTAL API.
+func SetAddrInfo(addrInfo AddrInfo, addr resolver.Address) resolver.Address {
+	newAddr := addr


Since this is passed by value, there's no need to make this copy here.

dfawley · 2020-04-22T17:01:46Z

balancer/weightedroundrobin/weightedroundrobin.go

+//
+// This is an EXPERIMENTAL API.
+func GetAddrInfo(addr resolver.Address) AddrInfo {
+	if addr.Attributes == nil {


Value handles nil *Attributes, so this check can be removed.

dfawley · 2020-04-22T17:06:37Z

balancer/weightedroundrobin/weightedroundrobin.go

+	if addr.Attributes == nil {
+		return AddrInfo{}
+	}
+	ai := addr.Attributes.Value(attributeKey{})
+	if ai == nil {
+		return AddrInfo{}
+	}
+	if _, ok := ai.(AddrInfo); !ok {
+		return AddrInfo{}
+	}
+	return ai.(AddrInfo)


Since everything is nil-pointer-safe, I think this function can be simplified to something like:

v := addr.Attributes.Value(attributesKey{}) ai, _ := v.(AddrInfo) // ignore ok and use zero value AddrInfo if v is nil return ai

Done. Thanks.

dfawley · 2020-04-22T17:13:48Z

xds/internal/balancer/edsbalancer/eds_impl.go

+				// attributes about the address. We still need to populate the
+				// Metadata field here to allow users of this field to migrate
+				// to the new one.
+				// TODO(easwars): Remove this once all users have migrated.


I'm a little lost. This is the address we're passing to the weighted round robin balancer - why do we need to set Metadata for it? Are they using xds and overriding our weighted RR balancer?

menghanl · 2020-04-22T23:13:53Z

balancer/weightedroundrobin/weightedroundrobin.go

+// with addrInfo.
+//
+// This is an EXPERIMENTAL API.
+func SetAddrInfo(addrInfo AddrInfo, addr resolver.Address) resolver.Address {


Nit: should addr come before addrInfo?
So this looks more like a method on resolver.Address, which it probably should be but we cannot do.

menghanl · 2020-04-22T23:15:24Z

balancer/weightedroundrobin/weightedroundrobin.go

+}
+
+// GetAddrInfo returns the AddrInfo stored in the Attributes fields of addr.
+// Returns nil if no AddrInfo is present.


This is not returning nil.

Should we just return the zero value? Or also return an OK?

It would be better if we can change Attributes.Value to return (interface{}, bool). So, all users of that package will benefit.

I don't understand - can you explain how this will help?

Given that the key for Attributes is local to the package, if Attributes.Value returned an extra return parameter indicating whether or not it found the key, the callers could be less paranoid when type asserting the returned interface{} and simply return a default value if the second return parameter is false.

The type assertion is safe, however, if you use the , ok form of it. And you'll get the zero value if the interface{} is nil. And if you do need it, you can rewrite:

v, ok := a.Value(...) if ok {

as

v := a.Value(...) if v != nil {

menghanl · 2020-04-22T23:15:55Z

balancer/weightedroundrobin/weightedroundrobin.go

+// This is an EXPERIMENTAL API.
+func GetAddrInfo(addr resolver.Address) AddrInfo {
+	v := addr.Attributes.Value(attributeKey{})
+	ai, _ := v.(AddrInfo)


This panics if the info is not in attributes.

Thanks. Done.

Are you sure? I don't think it will. The , _ should prevent the panic when v is nil.

https://play.golang.org/p/-DTj1hwQlPh

[facepalm] Does it have , _? My bad.

Wanted to add a test and make sure, but was lazy. Done now.

easwars · 2020-04-22T23:56:02Z

PTAL.

easwars · 2020-04-23T23:43:53Z

@dfawley : Please let me know if you have any more comments. Thanks.

easwars requested review from menghanl and dfawley April 15, 2020 00:28

easwars added no release notes Type: Feature New features or improvements in behavior labels Apr 15, 2020

menghanl suggested changes Apr 15, 2020

View reviewed changes

easwars assigned menghanl Apr 16, 2020

menghanl reviewed Apr 16, 2020

View reviewed changes

dfawley reviewed Apr 16, 2020

View reviewed changes

easwars commented Apr 16, 2020

View reviewed changes

dfawley reviewed Apr 17, 2020

View reviewed changes

dfawley reviewed Apr 22, 2020

View reviewed changes

dfawley assigned easwars Apr 22, 2020

easwars assigned dfawley and unassigned easwars Apr 22, 2020

menghanl reviewed Apr 22, 2020

View reviewed changes

menghanl approved these changes Apr 23, 2020

View reviewed changes

dfawley assigned easwars and unassigned menghanl and dfawley Apr 23, 2020

easwars assigned dfawley and unassigned easwars Apr 23, 2020

easwars added 4 commits April 23, 2020 16:46

Pass address weights to wrr balancer through attributes.

53e33db

Review comments #1.

fad5043

Review comments #2.

d77824f

Missed changing the eds_impl in the last commit.

0c70d5f

easwars added 7 commits April 23, 2020 16:46

Review comments #3.

f11e5ec

Make Clone work with nil Attributes.

9850c62

Get rid of Clone.

fcb660f

More review comments.

68320d3

more reviews.

77384ae

More review comments.

0a9c51d

Add note.

57ee0d2

easwars force-pushed the eds_wrr_attr branch from ab5bff6 to 57ee0d2 Compare April 23, 2020 23:47

dfawley approved these changes Apr 28, 2020

View reviewed changes

easwars merged commit b2df44e into grpc:master Apr 28, 2020

Iceber pushed a commit to Iceber/grpc-go that referenced this pull request Apr 29, 2020

Pass weights to wrr balancer through attributes. (grpc#3530)

5b1b50d

easwars deleted the eds_wrr_attr branch May 6, 2020 22:47

github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2021

Pass weights to wrr balancer through attributes. #3530

Pass weights to wrr balancer through attributes. #3530

Conversation

easwars commented Apr 15, 2020 • edited Loading

easwars commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Apr 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Apr 22, 2020

easwars commented Apr 23, 2020

easwars commented Apr 15, 2020 •

edited

Loading